Skip to content

CBL-6647: CBL sends no known ancestors in VV replication#2213

Merged
jianminzhao merged 10 commits intomasterfrom
CBL-6647
Sep 15, 2025
Merged

CBL-6647: CBL sends no known ancestors in VV replication#2213
jianminzhao merged 10 commits intomasterfrom
CBL-6647

Conversation

@callumbirks
Copy link
Contributor

@callumbirks callumbirks commented Jan 20, 2025

Also closes CBL-6521

(Addendum by @snej: description of CBL-6647 is "CBL should send an array of ancestors in response to ‘changes’ message. It seems that in VV replication the ancestors list is always empty. This means delta sync doesn’t work.")

@cbl-bot
Copy link

cbl-bot commented Jan 21, 2025

Code Coverage Results:

Type Percentage
branches 65.59
functions 78.12
instantiations 71.66
lines 77.12
regions 73.38

@pasin
Copy link
Collaborator

pasin commented Aug 13, 2025

Note: Based on my conversion with Adam, currently SG only expects the current version (CV) in the list of already-known ancestors when using version vector.

@pasin
Copy link
Collaborator

pasin commented Aug 22, 2025

I'm going to rebase this PR with the current master branch to get all of the PR validation to pass.

Copy link
Collaborator

@snej snej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the fix is correct. In fact I think our expected behavior isn't correct.

@snej
Copy link
Collaborator

snej commented Sep 10, 2025

The purpose of the ancestor revs sent in the changes response is to let the peer know how much history to send in its rev message, and to indicate what revs it could use as the base of a delta.

With VVs, the rev message always contains the entire VV (minus the CV because it's redundant with the rev property) so the first purpose isn't relevant.

For delta purposes, what we need to send in the response are just the current versions whose bodies we have. So we just send the local version and that of every remote that has a body. (Version, not version vector.)

I'll add a new commit that does this. It passes all unit tests, once I updated the expectations in the FindDocAncestors test.

It's only necessary for the method to return a list of Versions (not
vectors.) And it should send all the versions it has bodies for,
without trying to compare them against the remote vector.
@snej
Copy link
Collaborator

snej commented Sep 10, 2025

@jianminzhao Could you retest this PR against SG?

@jianminzhao
Copy link
Contributor

Merged in the latest master branch and verified with SG 4.0 that the PR fixed test if CBL-6521(https://jira.issues.couchbase.com/browse/CBL-6521)

@jianminzhao jianminzhao requested a review from snej September 15, 2025 21:00
@jianminzhao jianminzhao merged commit d7b4192 into master Sep 15, 2025
8 checks passed
@jianminzhao jianminzhao deleted the CBL-6647 branch September 15, 2025 22:47
jianminzhao added a commit that referenced this pull request Sep 16, 2025
CBL-6647: CBL sends no known ancestors in VV replication (#2213)
CBL-6649: Thread-safety problems with mbedTLS (#2348)
CBL-7427: Tests Against SG 4.0 (#2349)
CBL-5315: Reinstate flaky test "Continuous Push From Both Sides" for VV (#2345)
CBL-6666 CBL-6593: Fixed problems syncing partially VV-upgraded database [CBL-6666, CBL-6593] (#2335)
dcd4dab Fixed bug in Version::updateClock() (#2342)
CBL-7419: Fixed race condition calling Replicator::httpResponse() [CBL-7419] (#2343)
CBL-6969: Don't use env vars in CMake project (#2341)
CBL-6596: Update VS required version to 2 (#2340)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments